-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add let.Foo
sugar for continuation-passing-style
#2140
Conversation
@jaredly There are a couple reasons to factor out the "monad product" that is done by One is motivated by your let () = {
let!await name = getName();
let!await age = getAge(name);
{name, age}
} It's probably just a minor oversight, but the second let await = (value, continuation) => Js.Promise.then_(continuation, value);
let map = (value, continuation) => Js.Promise.map(continuation, value);
let () = {
let!await name = getName();
let!map age = getAge(name);
{name, age}
} If we want to be able to use Also, as can be seen from the option test case, in the PR as now, the user has to manually count how many let _ = {
let!opt x = Some(10);
let!opt2 a = Some(2) /* <--- If I add an and, do I need to make this opt3? */
and b = Some(5);
Some(a + x * b);
}; That seems like something best left to the compiler. |
First off, I'm totally willing to explore a solution for Yes it's less "elegant" than a solution with the monad product, but I think it's also less confusing, and so I'd like to try this way first. We can then later add a
It's boilerplate, but the best kind of boilerplate, in that it's trivial to get right, and probably won't need updating ever, because it lives in a library...
not sure what you mean by this.
If they're writing some huge composition, you could build up the product yourself.
It doesn't seem to me like having massive compositions will be common enough for us to make things more complicated to support it, at least for this initial pass.
Yup :) again, for this first pass I'm not too concerned about supporting support complex uses. We can add the All this to say:
|
I'm all for merging all or parts of this soon, in the interest of not holding up evolution, but just saying :) I do think the We could keep let await = {bind: (value, continuation) => ..., pair: (value, value') => ...} and having
I mean when a library gets things slightly wrong, like when the author didn't realize they have a monad (or applicative, or whatever) and didn't make it compatible with the syntax, and the user has to write some glue code. Another common case of this might be composed monads, like say there is an async monad that forgot to define an async+result monad, and now the user has to do it. This sort of thing comes up all the time in my OCaml experience. Also, on the subject of boilerplate, since result has two binds (on the value and the error), and two maps, that is that much more boilerplate to define if we have to bake in the products. Maybe the failure case is not important enough for making it compatible with the syntax, I don't know.
Yes. But the point where you have to do this is not predictable to the user. And I can imagine large |
Right now, I'm thinking we might want something like |
...and another option could be to define only |
Yeah, that does make the syntax simpler, but at the cost of making the transform more confusing. I like the simplicity of having the
tbh if you're but yeah, I think an |
yuuup that gets into rather more magic than I'd want |
I could imagine adding a "super-magic-monad" syntax later, but I want to keep this as straightforward and understandable to newcomers as possible, even though that means sacrificing some monady goodness. |
Somewhat tangential to the main point of this PR - can we avoid overloading the meaning of |
I chose |
or wait, what about like |
@jaredly I like it - It could be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can you add tests to make sure whitespace is interleaved correct?
let _ = {
let!opt x = Some(10);
let!opt2 a = Some(2)
and b = Some(5);
Some(a + x * b);
};
- Can you add tests with extensions?
let!await%lwt
}; | ||
|
||
let _ = { | ||
let!opt x = Some(10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add tests for the sugar outside of let bindings, e.g. structure_items, Pstr_eval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let bindings are the only place it's allowed. structure_items will throw an error (because there's no continuation, it doesn't really make sense).
I think I also want to disallow extensions... although I could be convinced
src/reason-parser/reason_attrs.ml
Outdated
uncurried : bool | ||
} | ||
end | ||
open T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid the open T
here?
I think a far more straightforward thing to do is to give Even simpler, we can reject |
yeah, that makes sense |
Ok, whitespace interleaving tested! |
@jaredly my apology for the barrage. Yes that's what I originally understood and back to understanding. I'll summarize. My concern (#2140 (comment)) is Your idea is a really neat generalization of It looks like the implementation still includes the @jordwalke's separate concern is how to handle async exceptions. I won't have time to write it up formally today but after a good sleep I agree. The desguaring could catch unhandled exceptions and re-raise them one level up. It'd be even nicer if every computation were stored in a |
I wanted to point out another use case for this proposal. PureScript and Scala use /* example */
let factors = n => {
0 -- n >>= a => /* for a in 0 to our number */
0 -- a >>= b => /* for b in 0 to a */
a * b == n >>? () => /* if the product equals our number */
return((a, b)) /* make a pair */
};
/* [[6,4],[8,3],[12,2],[24,1]] */
24 |> factors |> Js.log Using let factors = n => {
let%bind a = 0 -- n
let%bind b = 0 -- a
let%bind () = guard(a * b == n)
(a, b)
}; Using this proposal (I think) let yield = return
let for_ = bind
let if_ = (>>?)
let factors = n => { /* alternatively: */
let.for_ a = 0 -- n /* let a <= for_(0 -- n) */
let.for_ b = 0 -- a /* let b <= for_(0 -- a) */
let.if_ () = a * b == n /* let _ <= if_(a * b == n) */
yield((a, b))
}; Given open Belt.Array
/* ppx_let */
let return = x => [|x|]
let bind = (x, f) =>
reduce(x, [||], (acc, a) => concat(acc, f(a)))
/* MonadZero */
let guard = t => t ? [|()|] : [||]
/* infix */
let (>>=) = bind
let (>>?) = t => bind(guard(t))
let (--) = range |
@texastoland : You mentioned you haven't thought too much about the error handling. Aside from a nicer (imho) syntax than ppx-let, that error handling is the main reason I see this PR being better than ppx-let. If you have a proposal, please include how error propagation would work. Without it, a feature request is not really complete. The real problems come up when when handling errors and/or exceptions and it's that use case that I think this PR's justification becomes apparent. |
I don't see that being done here? I think it's a separate issue from the syntax being unintuitive (more importantly unequational) though.
I can write this up pretty trivially but I'd like to understand the status quo. |
Did you read the entirety of my Sketch? (It included an example in the linked Sketch that explains why this PR is helpful). |
Ah yes you articulated the problem awesome! I meant the current proposal doesn't address it either unless I misunderstood both the thread and this PR's changes. Regardless I agree and I'll publish a link concretely how I see it being desugared separate from any syntax concern. |
Hopefully not adding too much noise to this discussion, but I thought this alternative syntax might be interesting: let (x) = identifier(expr, $x); The idea here is to look somewhat similar to a regexp search and replace. It also allows you to put the callback argument in different places: let (x) = identifier($x, expr); Edit: this syntax won't work as is - but I believe it's still an interesting direction to explore. |
@SanderSpies that looks similar to @chenglou's proposal (comment) -- which was
For me, the ability to put the callback argument in different places substantially hurts "grokkability", because now I have to hunt for where the callback function is being inserted, which in a function that accepts multiple handlers (e.g. an "on success" callback and an "on error" callback) could drastically change the execution semantics of the following code. Your example also drops the The code sample (taken from reason-language-server, which there is using a variant of ppx_let) that I'm using to evaluate different syntax proposals is this: (and I'm having to guess a little bit what you mean by let (uri, position) = try(Protocol.rPositionParams(params), $(uri, position));
let (text, verison, isClean) =
try(maybeHash(state.documentText, uri) |> orError("No document text found"), $(text, verison, isClean));
let package = try(State.getPackage(uri, state), $package);
let offset =
try(PartialParser.positionToOffset(text, position) |> orError("invalid offset"), $offset);
let {extra} = try(State.getDefinitionData(uri, state, ~package), ${extra});
let position = Utils.cmtLocFromVscode(position);
{
let (commas, labelsUsed, lident, i) =
opt(PartialParser.findFunctionCall(text, offset - 1), $(commas, labelsUsed, lident, i));
let lastPos = i + String.length(lident) - 1;
let pos =
opt(PartialParser.offsetToPosition(text, lastPos) |?>> Utils.cmtLocFromVscode, $pos);
let (_, loc) = opt(References.locForPos(~extra, pos), $(_, loc));
let typ =
opt(switch (loc) {
| Typed(t, _) => Some(t)
| _ => None
}, $typ);
let rec loop = t => ...
let (args, rest) = loop(typ);
let args = opt(args == [] ? None : Some(args), $args);
Some(Ok(...));
} |? Ok((state, Json.Null)); How long does it take to spot all the And here's that code in the let.try (uri, position) = Protocol.rPositionParams(params);
let.try (text, verison, isClean) =
maybeHash(state.documentText, uri) |> orError("No document text found");
let.try package = State.getPackage(uri, state);
let.try offset =
PartialParser.positionToOffset(text, position) |> orError("invalid offset");
let.try {extra} = State.getDefinitionData(uri, state, ~package);
let position = Utils.cmtLocFromVscode(position);
{
let.opt (commas, labelsUsed, lident, i) =
PartialParser.findFunctionCall(text, offset - 1);
let lastPos = i + String.length(lident) - 1;
let.opt pos =
PartialParser.offsetToPosition(text, lastPos) |?>> Utils.cmtLocFromVscode;
let.opt (_, loc) = References.locForPos(~extra, pos);
let.opt typ =
switch (loc) {
| Typed(t, _) => Some(t)
| _ => None
};
let rec loop = t => ...;
let (args, rest) = loop(typ);
let.opt args = args == [] ? None : Some(args);
Some(Ok(...));
}
|? Ok((state, Json.Null)); |
@jaredly I guess you are right, in this example it's somewhat hard to spot the replace locations. Apologies for adding noise to the conversation, as it seems that Cheng's syntax version was in the same spirit and even better. The underscore has a different meaning for me, so that might be why I missed it. |
Tangentially related, with ppx_let I often find myself writing a series of
With the current proposal it would look like:
Which looks great! But maybe there's a room for a special syntax:
or something like that, which looks less noisy. |
So, thinking about this some more, and talking to @texastoland, I'm now thinking about it this way: If we want to do "plain functions", then the name of the function needs to go to the right of the Having "plain functions" where the name is next to the |
My idea was A friendlier option with more precedent would be to enclose the scope in a keyword (Haskell |
There's yet another advantage of using a module to represent semantics of CPS bindings. Consider this piece of code with ppx_let:
which will be turned by
(note the trailing
could be transformed into:
which won't lead to a type error. This will require to supply:
from authors of modules which work with such syntax. |
Hrmmmm I think I want to avoid magically adding a |
I made a ppx that implements the |
Ok, I've been using |
Tonight and tomorrow I'm writing up a thorough explanation in RFC form of @jaredly's implementation that answers "why not generalized CPS with arbitrary functions", "what about monads" (and how it relates to Haskell's |
Did you get around to drafting up an RFC, @texastoland? |
I outlined it and was working on code examples when I got wrapped up. Jared's |
Is there any further plan for this PR? I would like to stick to a certain strategy when dealing with async code in Reason. Thanks. |
I like this. It would be cool to build the syntax parsing changes which simply defines an extension point, then have the actual ast manipulation done as a separate ppx plugin so people can try it out and give feedback. That way we avoid breaking changes. I would really like to try this out in some of my projects. |
It would be much more easier (for me at least) to try the syntax (easier to implement too!) if it would desugar into ppx_let:
into
|
Any news around this? |
ocaml/ocaml#1947 got merged and Bob blogged about merging upstream changes. @jordwalke speculated about Reason implementing it ahead of BuckleScript. I assume the maintainers will wait for revisions (🤞🏽 see issue) to the official syntax. |
Can this be closed now or repurposed to backport the syntax like in Dune? |
As I think Reason will integrate the official |
closing in favor of #2487 |
This will allow us to get most of the benefits of lwt's ppx or ppx_let without baking in any specific implementation.
I also brought in some of the attribute handling changes in #1826 -- I didn't want to change too much, but it does make handling attributes much nicer.
thanks to @aantron for help with the implementation, and everyone who gave feedback & ideas on the concrete syntax.
Current (stripped-down) proposal
see bottom for original proposal
Potential future extensions
let.await
and auto-capitalize -- part of the original proposal, but removed for simplicity of initial implementation -- supported in comment by @jordwalkelet._
that references a locally definedlet_
instead of module-scoped: comment by @texastoland and comment by @hcartyswitch.Foo
that would map toFoo.switch_(arg, fn)
: comment by @hcartyand
s: comment by @texastolandReferences
original proposed syntax